Skip to content

LONDON| MAY 2025 | HAKAN MURAT KAVUT | Module-Structuring-and-Testing-Data - Sprint 3 #611

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

hmkavut
Copy link

@hmkavut hmkavut commented Jun 26, 2025

Learners, PR Template

Self checklist

  • I have committed my files one by one, on purpose, and for a reason
  • I have titled my PR with REGION | COHORT_NAME | FIRST_NAME LAST_NAME | PROJ_NAME
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

Changelist

Briefly explain your PR.

Questions

Ask any questions you have for your reviewer.

@hmkavut
Copy link
Author

hmkavut commented Jun 26, 2025

PASS Sprint-3/3-mandatory-practice/implement/get-ordinal-number.test.js
PASS Sprint-3/3-mandatory-practice/implement/repeat.test.js
PASS Sprint-3/2-mandatory-rewrite/3-get-card-value.test.js
PASS Sprint-3/2-mandatory-rewrite/1-get-angle-type.test.js
PASS Sprint-3/2-mandatory-rewrite/2-is-proper-fraction.test.js
PASS Sprint-3/3-mandatory-practice/implement/count.test.js

Test Suites: 7 passed, 7 total
Tests: 29 passed, 29 total
Snapshots: 0 total
Time: 0.705 s, estimated 1 s
Ran all test suites.

@hmkavut hmkavut added the Needs Review Participant to add when requesting review label Jun 26, 2025
Comment on lines +3 to +6
if (angle < 90) return "Acute angle";
if (angle > 90 && angle < 180) return "Obtuse angle";
if (angle === 180) return "Straight angle";
if (angle > 180 && angle < 360) return "Reflex angle";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec isn't clear whether angle can be assigned a number not in the interval (0, 360).
When angle is >= 360, what should the function return? (Also, by definition, angles <= 0 are not considered acute angles.)

When we implement a function that can return a value, to ensure reliability, we should ensure it will always return a defined value instead of undefined (which represents "no return value").
If the parameter, angle, is not within the recognised range, we can design the function to return a special value (e.g., "Invalid angle") or throw an error.

Comment on lines +2 to +4
if (Math.abs(numerator) < Math.abs(denominator)) return true;
if (Math.abs(numerator) > Math.abs(denominator)) return true;
if (Math.abs(numerator) === Math.abs(denominator)) return true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In mathematics, -A/B == A/-B == -(A/B), and -A/-B == A/B for any integers A and B (B ≠ 0).
They represent a proper fraction if A < B and A ≠ 0 and B ≠ 0.

The function is supposed to return true only when numerator/denominator represent a proper fraction.

@@ -5,7 +5,16 @@ test("should return true for a proper fraction", () => {
});

// Case 2: Identify Improper Fractions:
test("should return true for a improper fraction", () => {
expect(isProperFraction(5, 2)).toEqual(true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Identify improper fractions" means, "should return false for a improper fraction"

Comment on lines +18 to +20
test("should return true for a equal fraction", () => {
expect(isProperFraction(3, 3)).toEqual(true);
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3/3 is an improper fraction, so the function is expected to return false.

@@ -1,5 +1,8 @@
function getCardValue(card) {
// replace with your code from key-implement
return 11;
let rank = card[0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

card[0] only refers to the first character.

return "hellohellohello";
function repeat(str, count) {

if (Number(count) > 1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you explicitly convert count to a number? Isn't count already a number?

}
if (count == 1) return str;
if (count == 0) return "";
return "Negative counts are not valid.";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would the caller distinguish the result of the following two function calls?

  1. repeat("Negative counts are not valid.", 1)
  2. repeat("", -1)

Both function calls return the same value.

Comment on lines +45 to +50
test("should repeat empty string for count equals zero", () => {
const str = "hello";
const count = -5;
const repeatedStr = repeat(str, count);
expect(repeatedStr).toEqual("Negative counts are not valid.");
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you modified repeat() to throw an error when count is negative, and you wanted to test if the function can throw an error as expected, you can use .toThrow(). You can find out more about how to use .toThrow() here: https://jestjs.io/docs/expect#tothrowerror (Note: Pay close attention to the syntax of the example)

Comment on lines +6 to +10
let numbers = ['0', '1', '2', '3', '4', '5', '6', '7', '8', '9'];

// Build alphabet arrays manually
let smallLetters = [];
let upperLetters = [];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String also have includes() method which can be used to check if a character is one of the characters in the string.

We could also prepare numbers, smallLetters as

  // Declaring them using const can prevent them from being accidently reassigned a different value.
  const numbers = "0123456789";   
  const smallLetters = "abcdefghijklmnopqrstuvwxyz"; 

test("password has not number", () => {
// Arrange
const password = "HELLOWORLD#";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function can return false for multiple reasons.
To test a specific reason, choose an input that satisfies all other conditions except the one you're targeting. This way, if the function returns false, you can confidently attribute it to that specific condition.

For example, the function might return false for "HELLOWORLD#" not only because it lacks a digit, but also because it lacks a lowercase letter.
As a result, we can't be certain that the function correctly handles the case described in the test since multiple conditions are being violated simultaneously.

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review and removed Needs Review Participant to add when requesting review labels Jul 6, 2025
@cjyuan
Copy link
Contributor

cjyuan commented Jul 6, 2025

Can you also include a brief description of this PR in the "Changelist" section?
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Reviewed Volunteer to add when completing a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants